Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implemented PullRequest decoration to AzureDevOps server (issue/102 8 1support) #123

Closed
wants to merge 29 commits into from

Conversation

Iloer
Copy link

@Iloer Iloer commented Mar 12, 2020

#102 Implemented PullRequest decoration to AzureDevOps server
(only Git Repository, without TFVC)

mc1arke and others added 22 commits February 2, 2020 21:53
Sonarqube 8.1 removed the concept of short and long lived branches, instead simplifying the setup to either `BRANCH` or `PULL_REQUEST`. This release of Sonarqube also moved the management of Pull Request decoration into a standard User Interface calling a set of 'ALM' services that provide the management of decorators, and the binding of each project to these decorators. However the implementation of these services is not included in the Community Edition of Sonarqube, although the UI is made visible based on the presence of the branch management components provided by this plugin.

This change therefore introduces the services required to support UI components for the management of Pull Request decoration, as well as updating the handling of the configuration and loading of branches to support the removal of the SHORT/LONG branch constructs. As these changes require the reference of classes that were not present in older version of Sonarqube (namely `AlmSettingsDao` and `ProjectAlmSettingsDao`), the compatibility interfaces for these versions have been removed, as well as any methods and classes that existed purely to allow these versions to be supported by this plugin.

Given the standard UI provides a restricted set of fields for creating and binding each Pull Request decorator, some of the patterns that were previously used by this plugin for configuring the decoration of Pull Requests do not fit into this UI, and were awkward to find/manage when moved to another screen in the UI. This results in the configuration for disabling the addition and removing of comments on Merge Requests being removed from the plugin, with the Gitlab decorator removing the deleting of old comments since this was leading to discussion boxes being shown in Gitlab with no content, and the BitBucket decorator removing since it can't work out which user posted comments based purely on the configuration provided by Sonarqube.

To ensure the UI works for all configuration options, the services for configuring and binding Azure DevOps configuration have been included in this change-set, although no decorator currently exists for this ALM, so any project attempting to use this configuration will get a warning appear in the CE logs when attempting to use this decorator. Similarly, as the UI does not provide any options for configuring the URL for the Gitlab API, or the Slug/name of the project on the binding, a `Sensor` has been added into the scanner to detect these properties from injection by the Gitlab CI Runner, as well as injection directly from scanner arguments of `com.github.mc1arke.sonarqube.plugin.branch.pullrequest.gitlab.url` and `com.github.mc1arke.sonarqube.plugin.branch.pullrequest.gitlab.repositorySlug` for the repository URL and repository-name configuration entries.
The Github ALM Binding Web Service uses the `AlmRepo` field to store the repository name, but the Github decorator was using `AlmSlug` to try and retrieve the repository name, so was getting a `null` value back and failing to find a matching repository. Switching to using `AlmRepo` in the decorator overcomes this issues.
Initial commit with test
add getContextProperty method
Add POST PR Status
mc1arke#102 add Azure object class and small refactoring
mc1arke#102 add azure thread with issue
mc1arke#102 add: closed issue in sonar, resolve in Azure
mc1arke#102 small refactoring
remove unused code
remove star import
Copy link
Owner

@mc1arke mc1arke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution!

I've put a few comments on specific lines that generally apply to your whole change. Could we aim for immutable classes, and low visibility methods and fields where possible.

I've not gone through and functionally reviewed your changes yet given there are quite a few classes will need modified to meet the above ruquest.

Could you also squash your commits to a single commit per feature (i.e. this PR should probably be a single commit), and ensure your commit message follows https://github.com/RomuloOliveira/commit-messages-guide.

/**
* The ID of the parent comment. This is used for replies.
*/
public void setParentCommentId(Integer value){
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we make this object immutable?

Copy link
Author

@Iloer Iloer Mar 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resolved

import java.io.Serializable;

public class CommentPosition implements Serializable {
private Integer line;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

int rather than Integer, or is this nullalble?

Copy link
Author

@Iloer Iloer Mar 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resolved


public CommentPosition(Integer line, Integer offset){
this.line = line;
this.offset = offset + 1;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why + 1?

Copy link
Author

@Iloer Iloer Mar 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Char numbering in line in Azure PullRequest starts at "0", while in SonarQube starts at "1"

import com.fasterxml.jackson.annotation.JsonProperty;

public class CommentThreadResponse {
private CommentThread[] value;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

final

Copy link
Author

@Iloer Iloer Mar 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resolved

/**
* ID of the iteration to associate status with. Minimum value is 1.
*/
public Integer iterationId = null;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These should be private, and access through getters, with initialisation in the constructor

Copy link
Author

@Iloer Iloer Mar 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resolved

@@ -0,0 +1,5 @@
package com.github.mc1arke.sonarqube.plugin.ce.pullrequest.azuredevops.model;

public class PropertiesCollection {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the intent of this class? It contains no fields or methods

Copy link
Author

@Iloer Iloer Mar 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resolved

/**
* The thread status is unknown.
*/
unknown,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Naming conventions are to have enum fields in upper-case. Is there any reason that can't be followed here?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resolved

import com.github.mc1arke.sonarqube.plugin.ce.pullrequest.azuredevops.model.enums.CommentThreadStatus;
import org.sonar.api.issue.Issue;

import static org.sonar.api.issue.Issue.*;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please don't use star imports

Copy link
Author

@Iloer Iloer Mar 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resolved

import static org.sonar.api.issue.Issue.*;


public class CommentThreadStatusMapper {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

final if the constructor is private

Copy link
Author

@Iloer Iloer Mar 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resolved

import com.github.mc1arke.sonarqube.plugin.ce.pullrequest.azuredevops.model.enums.GitStatusState;
import org.sonar.api.ce.posttask.QualityGate;

public class GitStatusStateMapper {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

final if the constructor is private

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resolved

@Iloer
Copy link
Author

Iloer commented Mar 16, 2020

@mc1arke I am tried rebase, but something went wrong, please help squash the commits

mc1arke#102 enum to UPPERCASE
@bambuca
Copy link

bambuca commented Mar 20, 2020

Can it be used also with TFS on premise?

@Iloer
Copy link
Author

Iloer commented Mar 20, 2020

Yes, tested on AzureDevOps server 2019 (on premise) on API version 5.0-preview.1
Compatible with API version 4.0-preview
here is the mapping API and product versions

@Iloer
Copy link
Author

Iloer commented Mar 24, 2020

@mc1arke please see this pullrequets

@Iloer Iloer changed the title Issue/102 8 1support Implemented PullRequest decoration to AzureDevOps server (issue/102 8 1support) Mar 27, 2020
mc1arke#102 attempt to close comment in AzureDevOps by issue.key
@bambuca
Copy link

bambuca commented May 7, 2020

What about this PR? This is my dream PR 🥇

@mc1arke mc1arke force-pushed the sq-8_1-support branch 2 times, most recently from 11eac55 to 9b3aedf Compare May 17, 2020 17:24
@mc1arke mc1arke force-pushed the sq-8_1-support branch 3 times, most recently from 2d029de to 89b5982 Compare June 7, 2020 13:39
@mc1arke mc1arke closed this Jun 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants